Skip to content

Fixed wrong typing in AuthenticatedIdentity docBlocks #44

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 13, 2022
Merged

Fixed wrong typing in AuthenticatedIdentity docBlocks #44

merged 1 commit into from
Jun 13, 2022

Conversation

@ppaulis
Copy link
Contributor

@ppaulis ppaulis commented Jun 8, 2022

Signed-off-by: Pascal Paulis [email protected]

Q A
Documentation yes
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

Description

As the class currently already implements IdentityInterface (that in turn extends the RoleInterfaces from Rbac and Acl) and the class thus already possesses a method getRoleId() (as in the Acl RoleInterface), it seems to me that the concerned doc blocks should also use IdentityInterface instead of the Rbac RoleInterface.

The current version leads to errors in PHPStan (starting at --level 5 with v0.12.99) when doing :

new AuthenticatedIdentity($user); // $user implements Acl RoleInterface

Seems to have been introduced some months ago with PHP 8.0 support.
Concerned branches are 1.6.x and 1.7.x. 1.5.x doesn't have comments.

Thanks and best regards,
Pascal

@ppaulis ppaulis mentioned this pull request Jun 8, 2022
Closed
@Ocramius Ocramius added Bug Something isn't working Documentation labels Jun 10, 2022
@Ocramius Ocramius added this to the 1.7.1 milestone Jun 10, 2022
@Ocramius Ocramius self-assigned this Jun 10, 2022
Ocramius
Ocramius approved these changes Jun 10, 2022
View reviewed changes
Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ppaulis!

@Ocramius
Copy link
Member

Ocramius commented Jun 10, 2022

@ppaulis can you perhaps check if this reduces the psalm baseline, before we go ahead and 🚢?

@ppaulis
Copy link
Contributor Author

ppaulis commented Jun 13, 2022

@Ocramius thanks for the reminder! I checked, but unfortunately, nothing to remove in the baseline :-/

This one here could be easily removed:

<file src="src/Identity/AuthenticatedIdentity.php">
    <ImplementedReturnTypeMismatch occurrences="1">
      <code>null|string</code>
    </ImplementedReturnTypeMismatch>

by changing:

/** @return null|string */
public function getRoleId()
{

to:

/** @return string */
public function getRoleId()
{

but as the setter for $name allows setting null, this could potentially do more wrong than good.

@Ocramius
Copy link
Member

Ocramius commented Jun 13, 2022

@ppaulis that's fine, just wanted to check 👍

🚢

@Ocramius Ocramius merged commit 01d94f3 into laminas-api-tools:1.7.x Jun 13, 2022
@Ocramius Ocramius changed the title fixed wrong typing in doc blocks Fixed wrong typing in AuthenticatedIdentity docBlocks Jun 13, 2022
@github-actions github-actions bot mentioned this pull request Jun 13, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ocramius Ocramius Ocramius approved these changes

Assignees

@Ocramius Ocramius

Labels

Bug Something isn't working Documentation

Projects

None yet

Milestone

1.7.1

Development

Successfully merging this pull request may close these issues.

2 participants

@ppaulis @Ocramius